add minimal windows support #3
Conversation
| listenAddress () { | ||
| if (this._useFdPass) return 'http://*?fd=3'; | ||
| return this.url(); | ||
| } |
There was a problem hiding this comment.
You're still missing the point. This module is not meant to be Mojolicious specific at all. This could be a generic feature that any other web framework could use just the same.
| const connection = net.connect(port, resolve); | ||
| connection.on('error', err => { | ||
| if (err.code === 'ECONNREFUSED' && new Date() < timeToStop) setTimeout(loop, retryTime); | ||
| else resolve(); // this is intented: don't reject, just stop delaying |
There was a problem hiding this comment.
One of my requirements was that it rejects on errors. Are you sure you don't just want to make your own module?
| @@ -44,6 +44,7 @@ class Server extends EventEmitter { | |||
| this._process = undefined; | |||
| this._exitHandlers = []; | |||
| this._exit = undefined; | |||
| this._useFdPass = process.env.MOJO_SERVER_STARTER_AVOID_FDPASS ? false : process.platform !== 'win32'; | |||
There was a problem hiding this comment.
And let the users decide if they want to use a file descriptor or not. Hidden magic only makes it harder to write portable code.
| @@ -1,3 +1,3 @@ | |||
| node_modules | |||
| package-lock.json | |||
|
|
|||
| .vscode | |||
|
Opening a second PR feels like you're dismissing all my previous reviews. I've tried very hard to tell you exactly what i expect from this feature. And you still do the opposite intentionally (as illustrated by comments). I don't think both of us want the same from this module, so perhaps you would be better off just releasing your own Windows specific one instead. |
| setTimeout( | ||
| () => resolve(server.listen(listen)), | ||
| delay) | ||
| }))(); |
There was a problem hiding this comment.
I had renamed the test server to server_fd.js the other day specifically so the code doesn't have to get messy for new features.
|
Normally i would flag this PR as work in progress and wait for your response/changes, but since you close those anyway i'll do the same now. |
| if (this._useFdPass) resolve(); | ||
| else { | ||
| const now = new Date(); | ||
| const timeToStop = new Date(now.getTime() + connectTimeout); |
There was a problem hiding this comment.
Even my feedback on the unused now constant from the previous PR you've just dismissed without changes.
This should fix all pending claims from your review. Rebased from new master branch (PR#1 was based on windows branch, that's why I closed it and created this new one).
Related to issue #1